Avoid signed integer overflow in timelib_ts_at_start_of_year() #163
Conversation
We clamp the calculated timestamp to properly fit into a `timelib_sll` value. The properly supported range for `year` is [-292,277,022,656, 292,277,026,596] (if `long long` is 64bit).
|
I am not a fan of clamping results. I would rather see it continue to overflow, until an actual fix can be made (with large changes to timelib). There is also no test case associated with this patch, so I will be closing this PR. |
|
Note that signed overflow is undefined behavior, what can be really bad. That said, I agree that clamping isn't a solution, nor do I like the extensive checking throughout timelib, but php/php-src#17060 is likely worse than clamping, but more importantly isn't supported by all compilers. I'd be fine with comments for all relevant API functions clearly stating the supported value range (i.e. having documented preconditions). Clients could then check that upfront, and handle the error case instead of calling into timelib. |
2 participants
We clamp the calculated timestamp to properly fit into a
timelib_sllvalue.The properly supported range for
yearis [-292,277,022,656, 292,277,026,596] (iflong longis 64bit).See php/php-src#15150 (comment); this patch fixes the overflow in
timelib_ts_at_start_of_year(), although a follow-up overflow would happen intimelib_get_transitions_for_year(). Instead of having such checks all over the place, it seems worthwhile to consider to determine a generally supported range of valid years for whole timelib (that might depend onTIMELIB_SLL_MIN/TIMELIB_SSL_MAXthough).